MiqFileStorage interface and subclassing (with file splitting) #361

NickLaMuro · 2018-08-11T02:26:06Z

Update: Currently built off of #360 (will be implementing with that code soon)
Update: Also built off of #365. #360 might not stay as part of this PR, but currently including it here as well.
Update: Rebased in a lot of the [WIP]/[FIXUP] commits, and removed #360 from this PR (will open up a separate PR to implement FTP).

This introduces a higher level abstraction above MiqGenericMountSession which allows for a common API between mounted file systems, and what I have called in this branch name UploadTargets (might renamed to ObjectStorage).

Currently Implemented

MiqFileStorage and MiqFileStorage::Interface
- MiqFileStorage will be what is actually interacted with by the user (top level API still in the works)
- MiqFileStorage::Interface is what is the super class for MiqGenericMountSession and MiqObjectStore
Have MiqGenericMountSession inherit and conform to the MiqFileStorage::Interface
Introduced MiqLocalMountSession, which allows local files to also use this interface
File splitting support with MiqFileStorage::Interface classes

TODO

Possibly these will end up as follow up PRs, but regardless, the remaining TODO items for this repo is the following:

Add MiqObjectStore subclass
Move MiqS3Session to live under MiqObjectStore ~~(MiqS3ObjectStore maybe?)~~
- Update: Became MiqS3Storage

Decided to skip doing the following:

~~Implement the high level interface for MiqFileStorage (.add, .download, etc.)~~
~~Add MiqFtpObjectStore~~

NickLaMuro · 2018-08-11T02:29:16Z

@carbonin @Fryguy Sorry for the delay on getting this out, but this is what I got done so far. Will most likely be tackling the FTP stuff next. Still very much [WIP], but wouldn't mind your eyes and 🍅 's.

Will add some comments at some point soon around the tricky area's.

cc @jerryk55 @dmetzger57 @blomquisg

lib/gems/pending/util/miq_file_storage.rb

NickLaMuro · 2018-08-13T21:19:50Z

Note for future reference:

Looks like the travis failures are related to the following:

https://stackoverflow.com/questions/10238298/ruby-on-linux-pty-goes-away-without-eof-raises-errnoeio

Which is (briefly) mentioned in the sample code for PTY as well:

https://ruby-doc.org/stdlib-2.2.3/libdoc/pty/rdoc/PTY.html#module-PTY-label-Example

# The result of read operation when pty slave is closed is platform
# dependent.
ret = begin
        m.gets          # FreeBSD returns nil.
      rescue Errno::EIO # GNU/Linux raises EIO.
        nil
      end
p ret #=> nil

Will look at adding something like this to the code to confirm that it fixes the issue on Travis (which does run Debian containers if I am not mistaken.

Errors in Travis are very misleading because of the rescue/ensure code in and around #add in both MiqGenericMountSession and MiqFileStorage.

NickLaMuro · 2018-08-13T23:54:34Z

lib/gems/pending/util/mount/miq_local_mount_session.rb

+  # `remote_file_path` (might be everything if `remote_file_path` is a absolute
+  # path)
+  def mnt_point
+    @mnt_point ||= File.expand_path(remote_file_path).sub(remote_file_path, "")


After some thinking and looking at how to implement this in EvmDatabaseOps, this is a bad way to implement this via memoization.

Part of the reason to set @mnt_point was to prevent changes to MiqGenericMountSession, which doesn't use the accessor, but @mnt_point directly in a lot of spots. But, there are plenty of times where remote_file_path will not be set, and so generating @mnt_point in this fashion will cause issues (possibly just error out) if it is set and memoized before a #add call.

I have some ideas on how to do this better, but know that this implementation is probably going to change.

miq-bot · 2018-08-27T20:52:18Z

This pull request is not mergeable. Please rebase and repush.

lib/gems/pending/util/miq_file_storage.rb

carbonin · 2018-09-10T20:22:15Z

lib/gems/pending/util/miq_file_storage.rb

+    # pipe generated by this method and passed in to the block as a file
+    # location to the `input_stream`).
+    #
+    # In the non-block form, a source must be provioded as the first argument


Typo, s/provioded/provided/

lib/gems/pending/util/miq_file_storage.rb

carbonin · 2018-09-10T20:28:35Z

lib/gems/pending/util/miq_file_storage.rb

+
+    private
+
+    def upload_single(file) # rubocop:disable Lint/UnusedMethodArgument


Can you put a comment here describing how this should be implemented at a high level to work with splitting?

Yeah, good call.

I will probably point to one of the existing classes for examples, but yeah, this should definitely be documented.

carbonin · 2018-09-10T20:39:15Z

spec/util/mount/miq_local_mount_session_spec.rb

+      end
+    end
+
+    context "with slightly a slightly smaller input file than 10MB" do


Typo, remove the first "slightly"

carbonin · 2018-09-10T20:41:01Z

lib/gems/pending/util/mount/miq_local_mount_session.rb

+  # the portions of the expanded path that already exist in the
+  # `remote_file_path` (might be everything if `remote_file_path` is a absolute
+  # path)
+  def mnt_point


This logic looks a bit complicated. Should we test this directly or you you think this is exercised enough through the existing tests?

It is exercised (I believe) in the MiqLocalMountSession tests, as I switch up what I pass to .add in each of the different tests.

But I can target it specifically in that spec if you want (seems like it wouldn't be a bad idea).

I think a spec for this would be good. If nothing else it would be a good descriptor of what this method is actually doing.

Actually, after doing some investigating, I am going to try deleting this code and use a slightly different approach. I will report back, but that might be the easier route.

lib/gems/pending/util/miq_object_storage.rb

carbonin · 2018-09-10T20:45:05Z

lib/gems/pending/util/extensions/aws-sdk/s3_upload_stream_patch.rb

+
+require 'aws-sdk'
+
+if defined? Aws::S3::GEM_VERSION # This constant doesn't exist in v2


So, I wouldn't want to push it just for this patch, but do we know if we are super far away from being able to use the new version as a whole? Would be curious to know how much effort that would be rather than forward porting this patch. @bronaghs Can you comment here or refer who would know about the aws-sdk version we are using?

do we know if we are super far away from being able to use the new version as a whole?

I don't know personally, or if it has been considered at all.

I want to say there was something that was being done to update fog in the recent future, and I thought that was potentially going to update the aws-sdk, but I might be remembering incorrectly (Update: I was wrong... no surprise... It was for ManageIQ/manageiq#17258 and it was fog-google, not AWS... oops)

Switching to v3 of the amazon API is not in the near future... cc @bronaghs @agrare

Looks like the far future has arrived? #374

carbonin · 2018-09-10T20:46:20Z

lib/gems/pending/util/object_storage/miq_s3_storage.rb

+    super(settings)
+
+    # NOTE: This line to be removed once manageiq-ui-class region change implemented.
+    @settings[:region] ||= "us-east-1"


Since we're here anyway ... @jerryk55 can we resolve this NOTE:?

From my take, I am not sure if it is a bad thing to keep, and we might just be able to delete the comment. I simply ported over the comment from the old code since it seemed important.

But I would like @jerryk55 's opinion before I do.

Okay, but if we're keeping it, then we can probably drop the comment. Waiting on your input here @jerryk55

NickLaMuro · 2018-09-11T05:28:43Z

@carbonin I believe that I addressed most of your comments.

carbonin · 2018-09-11T18:29:40Z

lib/gems/pending/util/miq_file_storage.rb

+    scheme, _ = URI.split(URI::DEFAULT_PARSER.escape(opts[:uri]))
+    klass = storage_interface_classes[scheme]
+
+    raise ArgumentError, "Not a valid uri!" if klass.nil?


I think we can be more descriptive here. Maybe something like:

raise ArgumentError, "#{scheme} is not a valid MiqFileStorage uri scheme. Accepted schemes are #{storage_interface_classes.keys}" if klass.nil?

carbonin

Just one last minor comment.

Everything else looks good to me. The only things I think are still open on this PR are attention from @agrare or @bronaghs about the aws-sdk gem and input from @jerryk55 about the region default, but I'm willing to merge after this last comment is addressed.

carbonin · 2018-09-12T13:02:42Z

lib/gems/pending/util/miq_file_storage.rb

+               end
+      # `.join` will raise any errors from the thread, so we want to do that
+      # here (if a thread exists of course).
+      thread.join if block_given?


I think this would be more clear if we used the thread's existence as the condition rather than if the block was provided.

It's a bit confusing that I have to look into #handle_io_block to figure out that a thread is only created if a block is provided.

Sure, makes sense to me. I think I was just using that as the check elsewhere, but I think that is a reasonable change.

`MiqFileStorage` is meant to be an top level class for fetching classes that interface with `MiqFileStorage::Interface`. `MiqFileStorage::Interface` is meant to be the super class for all of the class to inherit from, to make sure that they conform to a spec that `MiqFileStorage` can work with.

This mount session is intended as being a pass through for when local files are used. The same API's for `#add` from MiqGenericMountSession are still usable for this class, and it will just write to the local file system instead. This also allows us to test the file splitting code without needing a mount session in place to do so.

This is the top level class for Object Storage based file storages, which will include types like s3 and FTP. Doesn't do much, but shared methods between all of the Object Storage classes will be put here.

Converts MiqS3Session to an MiqObjectStorage subclass, MiqS3Storage. A decent amount here didn't change, but it updates the `#add` and `#download` methods to use the MiqFileStorage::Interface and instead override `#upload_single` and `#download_single`. Many of the tests didn't make it over since a lot of it was in regards to "mounting", which ObjectStorage classes don't do and is now obsolete, as well as cleaned up MiqGenericMountSession from any references that were s3 specific. Also dried up the code a bit from what was implemented in MiqS3Session. Regarding the elephant in the room... ------------------------------------- Yes, I backported Aws::S3::MultipartStreamUploader from the v3 portion of the aws-sdk as part of this commit. Some form of "upload streaming" was necessary for file splitting to work. I could have implemented something myself with probably a little less code, but what was already part of the v3 API, is probably tested extensively, and works with the v2 Seahorse client and lib made more sense to implement via this way instead of trying to role my own. It is a lot of extra code, but I have a flag in place for when we do upgrade to aws-sdk v3 in the future, an error should be raised in the test suite to have this patch removed.

miq-bot · 2018-09-12T15:47:53Z

Checked commits NickLaMuro/manageiq-gems-pending@1c5c8c5~...f3e3e9c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
12 files checked, 2 offenses detected

lib/gems/pending/util/miq_file_storage.rb

❗ - Line 120, Col 7 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

lib/gems/pending/util/object_storage/miq_s3_storage.rb

❗ - Line 96, Col 3 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

carbonin

Great work here @NickLaMuro

NickLaMuro · 2018-09-21T14:56:11Z

lib/gems/pending/util/object_storage/miq_s3_storage.rb

+
+  def s3
+    require 'aws-sdk'
+    require 'util/extensions/aws-sdk/s3_upload_stream_patch'


Bah... this means that this didn't get loaded in the specs... which would have made things easier...

Oh well I guess, we caught the PR updating this, so I guess all is good.

miq-bot added the wip label Aug 11, 2018

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch 3 times, most recently from 9362637 to 5255d29 Compare August 13, 2018 17:45

carbonin reviewed Aug 13, 2018

View reviewed changes

lib/gems/pending/util/miq_file_storage.rb Show resolved Hide resolved

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch 2 times, most recently from f74ed02 to 1e77e77 Compare August 13, 2018 21:07

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch 3 times, most recently from abfab9b to 8476d86 Compare August 13, 2018 22:01

NickLaMuro commented Aug 13, 2018

View reviewed changes

NickLaMuro mentioned this pull request Aug 14, 2018

[WIP] Update upload/download workflow for MiqS3Session ManageIQ/manageiq#17798

Closed

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch 6 times, most recently from 1b70fc5 to c9dc4dc Compare August 20, 2018 17:33

NickLaMuro mentioned this pull request Aug 20, 2018

Swift DB Backups Requirements ManageIQ/manageiq-schema#259

Merged

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch from c9dc4dc to 3ee8fb6 Compare August 22, 2018 06:03

This was referenced Aug 22, 2018

Add file splitting to evm:db tasks (V2) ManageIQ/manageiq#17894

Merged

Use STDOUT for pg_basebackup #365

Merged

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch 3 times, most recently from 34302a5 to aea0477 Compare August 24, 2018 18:19

miq-bot added the unmergeable label Aug 27, 2018

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch from aea0477 to ca81761 Compare August 28, 2018 16:32

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch 3 times, most recently from 5a500d3 to 5f5158e Compare September 5, 2018 18:55

NickLaMuro changed the title ~~[WIP] MiqFileStorage interface and subclassing (with file splitting)~~ MiqFileStorage interface and subclassing (with file splitting) Sep 5, 2018

miq-bot removed the wip label Sep 5, 2018

NickLaMuro mentioned this pull request Sep 10, 2018

DB Backups to Openstack Swift #371

Merged

carbonin reviewed Sep 10, 2018

View reviewed changes

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch from 5f5158e to ab70c42 Compare September 11, 2018 05:22

This comment has been minimized.

Sign in to view

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch from ab70c42 to 5b2d047 Compare September 11, 2018 16:23

carbonin reviewed Sep 11, 2018

View reviewed changes

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch 2 times, most recently from 7d5d866 to 4877422 Compare September 11, 2018 22:40

carbonin reviewed Sep 12, 2018

View reviewed changes

carbonin self-assigned this Sep 12, 2018

carbonin added enhancement gaprindashvili/no labels Sep 12, 2018

NickLaMuro added 5 commits September 12, 2018 10:43

Update MiqGenericMountSession to MiqFileStorage::Interface

e1b467b

Add MiqObjectStore

d79deab

This is the top level class for Object Storage based file storages, which will include types like s3 and FTP. Doesn't do much, but shared methods between all of the Object Storage classes will be put here.

NickLaMuro force-pushed the file_storage_and_upload_target_abstractions branch from 4877422 to f3e3e9c Compare September 12, 2018 15:44

carbonin approved these changes Sep 12, 2018

View reviewed changes

carbonin merged commit 7ba5665 into ManageIQ:master Sep 12, 2018

carbonin added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 12, 2018

NickLaMuro mentioned this pull request Sep 19, 2018

Adds file splitting support to dump/backup commands ManageIQ/manageiq-appliance_console#50

Merged

3 tasks

NickLaMuro commented Sep 21, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MiqFileStorage interface and subclassing (with file splitting) #361

MiqFileStorage interface and subclassing (with file splitting) #361

NickLaMuro commented Aug 11, 2018 •

edited

Loading

NickLaMuro commented Aug 11, 2018

NickLaMuro commented Aug 13, 2018 •

edited

Loading

NickLaMuro Aug 13, 2018

miq-bot commented Aug 27, 2018

carbonin Sep 10, 2018

carbonin Sep 10, 2018

NickLaMuro Sep 10, 2018

carbonin Sep 10, 2018

carbonin Sep 10, 2018

NickLaMuro Sep 10, 2018

carbonin Sep 11, 2018

NickLaMuro Sep 11, 2018

carbonin Sep 10, 2018

NickLaMuro Sep 11, 2018 •

edited

Loading

Fryguy Sep 11, 2018

carbonin Sep 21, 2018

carbonin Sep 10, 2018

NickLaMuro Sep 10, 2018

carbonin Sep 11, 2018

This comment has been minimized.

NickLaMuro commented Sep 11, 2018

carbonin Sep 11, 2018

carbonin left a comment

carbonin Sep 12, 2018

NickLaMuro Sep 12, 2018

miq-bot commented Sep 12, 2018

carbonin left a comment

NickLaMuro Sep 21, 2018


		private

		def upload_single(file) # rubocop:disable Lint/UnusedMethodArgument


		require 'aws-sdk'

		if defined? Aws::S3::GEM_VERSION # This constant doesn't exist in v2

MiqFileStorage interface and subclassing (with file splitting) #361

MiqFileStorage interface and subclassing (with file splitting) #361

Conversation

NickLaMuro commented Aug 11, 2018 • edited Loading

Currently Implemented

TODO

NickLaMuro commented Aug 11, 2018

NickLaMuro commented Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

NickLaMuro commented Sep 11, 2018

Choose a reason for hiding this comment

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 12, 2018

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Aug 11, 2018 •

edited

Loading

NickLaMuro commented Aug 13, 2018 •

edited

Loading

NickLaMuro Sep 11, 2018 •

edited

Loading